Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

store/tikv: add sanity check for startTS in 2pc #9555

Merged
merged 2 commits into from
Mar 5, 2019
Merged

store/tikv: add sanity check for startTS in 2pc #9555

merged 2 commits into from
Mar 5, 2019

Conversation

disksing
Copy link
Contributor

@disksing disksing commented Mar 5, 2019

Signed-off-by: disksing [email protected]

What problem does this PR solve?

To avoid committing a transaction with MAX_UINT64 unexpectedly.

What is changed and how it works?

Sometimes (PointGet, Analyze) TiDB uses special startTS (MAX_UINT64) to initialize a transaction. This is dangerous because txn.Commit() may be called unexpectedly.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jackysp
Copy link
Member

jackysp commented Mar 5, 2019

Related to #9283

@shenli
Copy link
Member

shenli commented Mar 5, 2019

@disksing Please fix the CI.
Please cherry-pick this to the release-2.x.

@shenli shenli added the sig/transaction SIG:Transaction label Mar 5, 2019
Signed-off-by: disksing <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #9555 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9555      +/-   ##
==========================================
- Coverage   67.37%   67.36%   -0.02%     
==========================================
  Files         375      375              
  Lines       78779    78783       +4     
==========================================
- Hits        53081    53075       -6     
- Misses      20927    20935       +8     
- Partials     4771     4773       +2
Impacted Files Coverage Δ
store/tikv/2pc.go 77.7% <100%> (-0.92%) ⬇️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
ddl/session_pool.go 82.75% <0%> (-10.35%) ⬇️
executor/distsql.go 71.72% <0%> (-0.46%) ⬇️
ddl/delete_range.go 76.71% <0%> (+1.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b49c9...a5f0994. Read the comment docs.

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 5, 2019
Copy link
Member

@shenli shenli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shenli
Copy link
Member

shenli commented Mar 5, 2019

/run-all-tests

@disksing disksing added status/all tests passed status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Mar 5, 2019
@disksing disksing merged commit d7ac113 into pingcap:master Mar 5, 2019
@disksing disksing deleted the max-ts branch March 5, 2019 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/transaction SIG:Transaction status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants